mcp: handle oauth2.RetrieveError in client authorization retry logic#909
mcp: handle oauth2.RetrieveError in client authorization retry logic#909smlx wants to merge 1 commit intomodelcontextprotocol:mainfrom
Conversation
Previously, an expired refresh token in the oauth2.Token returned from OAuthHandler.TokenSource() would cause the connection to fail. From the client perspective, this meant that the MCP connection was in a hard-failed state with no way to re-authorize. The change in this commit causes Authorize() to be called in the event of both an oauth2.RetrieveError, as well as in the pre-existing case of a 401/403 HTTP response. Clients will handle this in their existing Authorize() flows to get a new valid token for the connection.
|
Could you explain the user journey in more detail, preferably as a list of events? Is it about a situation where the token from authorization was not used for a long time (maybe written to and loaded from disk after a significant time) so that the refresh token is no longer valid? |
Yes, exactly.
Events 5 and 6 happen here: Lines 1910 to 1923 in 93a41b2 Before this change, the After this change, the new
|
|
|
||
| var authResp *http.Response | ||
| var retrieveErr *oauth2.RetrieveError | ||
| if err != nil && errors.As(err, &retrieveErr) { |
There was a problem hiding this comment.
I'm wondering if the check is too broad. Should we only be re-authorizing if retriveErr.ErrorCode == "invalid_grant"?
The provided authorization grant (e.g., authorization code, resource owner credentials) or refresh token is invalid, expired, revoked, does not match the redirection URI used in the authorization request, or was issued to another client.
There was a problem hiding this comment.
Yes I think you are right. I'll fix this up.
| // | ||
| // doRequest should construct and send the HTTP request, and return the sent request (which | ||
| // may be needed for authorization), the response (if any), and any error. | ||
| func (c *streamableClientConn) doWithAuth(ctx context.Context, requestSummary string, doRequest func() (*http.Request, *http.Response, error)) (*http.Request, *http.Response, error) { |
There was a problem hiding this comment.
nit: let's make doRequest take context.Context to avoid unintentional wrong context capture
| return req, resp, err | ||
| } | ||
|
|
||
| if authErr := c.oauthHandler.Authorize(ctx, req, authResp); authErr != nil { |
There was a problem hiding this comment.
looking into Authorize it relies on authResp to have WWW-Authenticate set. if fallbacks to well-known won't work the whole Authorize will fail.
might be a bit hacky, but what if we handle oauth2.RetrieveError earlier by just not setting "Authorization" header and letting request fail with 401/403?
There was a problem hiding this comment.
looking into Authorize it relies on authResp to have WWW-Authenticate set. if fallbacks to well-known won't work the whole Authorize will fail.
I see what you are saying, but how likely/possible is this? Seems to defeat the purpose of being "well-known" 😕
might be a bit hacky, but what if we handle oauth2.RetrieveError earlier by just not setting "Authorization" header and letting request fail with 401/403?
That is an interesting idea. In fact, it might be a better solution than this PR. I'll look into it.
There was a problem hiding this comment.
I see what you are saying, but how likely/possible is this? Seems to defeat the purpose of being "well-known"
I don't know, but it is possible. The spec says:
MCP servers MUST implement one of the following discovery mechanisms to provide authorization server location information to MCP clients:
- WWW-Authenticate Header: ...
- Well-Known URI: ...
So the failed empty-auth request approach should be more reliable. It's also a bit easier to reason about in the sense that Authorize only receives MCP (not auth) server response.
@maciej-kisiel and I also considered how oauth.Config which token refresher already has can be passed to Authorize for skipping the discovery step in re-authorization, but all the options seem not worth it (in terms of code complexity) if a single failed request can handle this edge case.
3 participants
Previously, an expired refresh token in the oauth2.Token returned from OAuthHandler.TokenSource() would cause the connection to fail.
From the client perspective, this meant that the MCP connection was in a hard-failed state with no way to re-authorize.
The change in this commit causes Authorize() to be called in the event of both an oauth2.RetrieveError, as well as in the pre-existing case of a 401/403 HTTP response. Clients will handle this in their existing Authorize() flows to get a new valid token for the connection.